Skip to content

Release v0.4.2#12

Merged
joshwilhelmi merged 8 commits into
mainfrom
0.4.2
May 12, 2026
Merged

Release v0.4.2#12
joshwilhelmi merged 8 commits into
mainfrom
0.4.2

Conversation

@joshwilhelmi
Copy link
Copy Markdown
Contributor

@joshwilhelmi joshwilhelmi commented May 12, 2026

Summary

Validation

  • uv run gobby restart
  • uv run gobby health
  • GOBBY_TEST_PROTECT=1 uv run pytest tests/cli/test_install_embedding_wizard.py tests/cli/installers/test_embedding_installer.py tests/test_build_backend.py tests/servers/test_app_factory_production_ui.py -v
  • uv run ruff check src/
  • uv run ruff format --check src/
  • GOBBY_TEST_PROTECT=1 uv run mypy src/ --no-incremental --strict
  • GOBBY_SKIP_UI_BUILD=1 uv build
  • git push pre-push hooks: lint, format, type_check, ts_check, frontend_tests

Summary by CodeRabbit

Release Notes v0.4.2

  • New Features

    • Added CLI flags (--embedding-url, --embedding-model, --embedding-dim) to configure custom OpenAI-compatible embedding endpoints during installation with automatic dimension detection.
  • Bug Fixes

    • Fixed web UI packaging in distributions.
    • Fixed MCP server configuration migration to new location.
  • Documentation

    • Updated web UI port references: installed UI runs on :60887, dev mode on :60889.
    • Updated MCP configuration location to mcp-servers.json.

Review Change Stack

joshwilhelmi and others added 8 commits May 11, 2026 12:55
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Voice tests run in a dedicated Voice Extra job (ci.yml:174) with the
voice extra installed. The main Test (Python 3.13/3.14) and release
test jobs were re-running them without the extra, which caused 3.13
to spend ~20+ minutes loading TTS/STT models late in the run.

Add --ignore=tests/voice and --ignore=tests/servers/routes/test_voice_routes.py
to the main pytest invocations. Coverage on voice code is still
collected through the Voice Extra job.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes GH #10. `uv tool install gobby` previously left the web UI
unreachable: `:60887/` 404'd because the daemon's `_mount_production_ui`
couldn't find UI assets, and `:60889` was never listening because no
production process serves it. The root cause was that the wheel's
`package-data` glob did not include `web/dist/` and `find_web_dir()`
required `package.json` (which the installed wheel never ships).

A second bug surfaced alongside: `gobby ui *` CLI commands loaded
config via `load_config()` without a `ConfigStore`, so flipping
`ui.enabled=true` in the DB was invisible to the CLI - only the
daemon saw it.

Changes:
- New `build_backend/` PEP 517 wrapper that runs `npm ci && npm run build`
  in `web/` and copies `web/dist/` into `src/gobby/ui/web/dist/`
  before wheel packaging. `GOBBY_SKIP_UI_BUILD=1` escape hatch for
  CI; falls back to pre-staged dist if npm is unavailable.
- `pyproject.toml`: register the wrapper and add `ui/web/dist/**/*`
  to the `gobby` package-data glob.
- `find_web_dir()` now accepts dist-only install-mode layouts; a new
  `require_source=True` flag keeps `gobby ui dev/build/install-deps`
  on the source-mode (package.json) path.
- `load_full_config_from_db()` takes an optional `config_file`; the
  CLI root uses it so every subcommand sees DB-resolved config.
- `cli/ui.py`: drop the broken `WEB_UI_DIR` constant; resolve via
  `find_web_dir(config, require_source=True)`.
- Tests: production-mount integration tests, install-mode
  `find_web_dir` coverage, build-backend staging tests, end-to-end
  regression test for `ui.enabled` read-through, and updated the
  CLI test suite for the patch-target rename.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
setuptools is a build-time dependency, not a runtime one. The earlier
eager `from setuptools import build_meta as _orig` broke any code that
imports the `build_backend` module at runtime - including the wrapper's
own focused unit tests - in an env where setuptools isn't installed.

Move the setuptools import behind a small `_orig()` accessor and a
module-level `__getattr__` that forwards the PEP 517 hook attributes
(`get_requires_for_*`, `prepare_metadata_for_*`) on demand. The actual
build hooks (`build_wheel`, `build_sdist`, `build_editable`) call
`_orig()` only when invoked, which is the only time setuptools is
guaranteed to be present anyway.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes GH #11. Gobby's home-dir MCP server registry collided with Claude
Code's project-level `.mcp.json` schema; running Claude Code from `~`
(or any tool that walks for `.mcp.json` with the Claude Code parser)
would emit a parse error against Gobby's `{"servers": [...]}` shape.

- Rename the file to `~/.gobby/mcp-servers.json`. New module-level
  constants `DEFAULT_MCP_CONFIG_PATH` and `LEGACY_MCP_CONFIG_PATH` in
  `gobby.config.mcp` hold the canonical paths.
- Add idempotent `migrate_legacy_mcp_config()` that renames the legacy
  file when the new one is missing, logs a warning when both exist,
  and is a no-op otherwise.
- Call the migrator from `MCPConfigManager.__init__`, from
  `install_default_mcp_servers()`, and once early in daemon startup
  (`run_daemon`) so existing installs upgrade transparently.
- Update the config docs to reference the new filename.
- The in-isolation `.mcp.json` written by `agents/isolation.py`
  intentionally uses Claude Code's schema and is left alone.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…stall

Fixes GH #9. The embedding installer hardcoded `api_base`, `model`,
and `dim` per provider, so users running LM Studio on a non-default
host/port - or wanting a stronger model like Qwen3-Embedding-4B - had
to hand-edit `~/.gobby/gobby-hub.db` to override `embeddings.api_base`,
`embeddings.model`, and `embeddings.dim`.

- `gobby install` now accepts `--embedding-url`, `--embedding-model`,
  and `--embedding-dim`. Each overrides the matching field on whichever
  provider was selected.
- When the user supplies a custom URL or model but omits `--embedding-dim`,
  the installer probes `/v1/embeddings` with a 1-token request and reads
  `len(data[0].embedding)` to detect the dim automatically.
- When the user supplies `model_override`, the LM Studio / Ollama
  bundled-model setup is skipped - they're bringing their own model
  (e.g. a Qwen3-Embedding GGUF served by LM Studio), and we should not
  try to `lms get nomic` underneath them.
- The setup wizard exposes the same three knobs: after picking a
  provider it asks "Customize endpoint URL, model id, or embedding
  dim?" and collects whichever fields the user wants to override.
  Blank dim triggers the probe path.
- The default remains nomic-embed-text-v1.5@f16 (768-dim) when no
  flags are passed.
- Docs add a Qwen3-Embedding-4B recommendation with the storage and
  latency tradeoff note.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 12, 2026 02:10
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 12, 2026

Note

.coderabbit.yaml has unrecognized properties

CodeRabbit is using all valid settings from your configuration. Unrecognized properties (listed below) have been ignored and may indicate typos or deprecated fields that can be removed.

⚠️ Parsing warnings (1)
Validation error: Unrecognized key(s) in object: 'tools', 'ignore'
⚙️ Configuration instructions
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
📝 Walkthrough

Walkthrough

Version 0.4.2 is a patch release that adds web UI wheel packaging via a custom PEP 517 build backend, embedding endpoint customization with CLI flags and dimension probing, MCP server config migration to a new path with idempotent legacy handling, and CLI refactoring to improve config-loading and web-directory discovery. Documentation is updated to clarify port usage and new configuration paths.

Changes

Web UI Packaging and Build Backend

Layer / File(s) Summary
Build backend wrapper and UI staging
build_backend/__init__.py
Implements PEP 517 backend wrapper that stages web UI before wheel/sdist builds; runs npm build in web/, copies dist/ into src/gobby/ui/web/dist/, and validates wheel contains gobby/ui/web/dist/index.html; uses lazy setuptools import and delegates non-wrapped hooks.
Build system configuration and manifest
pyproject.toml, MANIFEST.in
Updates build backend from setuptools.build_meta to custom build_backend with backend-path=".", includes ui/web/dist/**/* in package data, and adds build_backend/__init__.py to source distribution manifest.
CI workflow wheel and package validation
.github/workflows/ci.yml, .github/workflows/release.yml
Validates built wheels contain gobby/ui/web/dist/index.html, inspects both source and wheel distributions, filters out voice tests from pytest runs.
Build backend unit tests and production UI mounting tests
tests/test_build_backend.py, tests/servers/test_app_factory_production_ui.py
Unit tests covering UI staging from web/dist, env-var skip, pre-staged reuse, wheel verification; separate tests for production UI mounting covering index serve, asset serving, SPA fallback, and no-op behavior.

Embedding Endpoint Customization

Layer / File(s) Summary
CLI install flags and prompt infrastructure
src/gobby/cli/install.py, src/gobby/cli/_install_prompts.py
Adds --embedding-url, --embedding-model, --embedding-dim CLI options; implements _infer_embedding_provider_from_url helper; extends embedding prompts with interactive customization flow for URL/model/dimension overrides.
Embedding installer with override support and dimension probing
src/gobby/cli/installers/embedding.py
Updates install_embedding to accept override parameters; skips bundled setup when overrides provided; adds _probe_embedding_dim helper to auto-detect dimension via short embedding request when not explicitly specified.
Embedding installer and wizard override tests
tests/cli/installers/test_embedding_installer.py, tests/cli/test_install_embedding_wizard.py
Tests override behavior covering explicit overrides, API-base-only with probing, combined model+endpoint, failed probe with actionable error; wizard tests for CLI override flow, custom URL provider inference, and interactive override collection.

MCP Server Config Migration and Daemon Startup

Layer / File(s) Summary
MCP config module with legacy migration
src/gobby/config/mcp.py
Adds DEFAULT_MCP_CONFIG_PATH, LEGACY_MCP_CONFIG_PATH constants and migrate_legacy_mcp_config function for idempotent file-rename migration; updates MCPConfigManager to call migration and default to new path.
MCP installer and daemon startup migration hooks
src/gobby/cli/installers/mcp_config.py, src/gobby/runner_lifecycle.py
Updates install_default_mcp_servers to use new path and invoke migration; adds startup migration in run_daemon with try/except error handling.
MCP config migration tests
tests/config/test_mcp_config.py
Tests for migrate_legacy_mcp_config covering rename scenarios, no-op behavior, and parent directory creation; updates default path expectation to mcp-servers.json.

CLI Config Loading Refactor and Web Directory Discovery

Layer / File(s) Summary
CLI entry point and config loading refactor
src/gobby/cli/__init__.py
Switches from load_config to load_full_config_from_db for layered config resolution; stores resolved config in ctx.obj["config"] for subcommand access.
Web directory discovery and UI command refactoring
src/gobby/cli/ui.py, src/gobby/cli/utils.py
Adds _resolve_source_web_dir helper; updates dev, build, install_deps commands to use improved find_web_dir with require_source flag; find_web_dir now supports both source (package.json) and production (dist/index.html) layouts.
UI command tests with web directory mocking
tests/cli/test_ui_coverage.py
Updates dev, build, install_deps test suites to mock find_web_dir instead of WEB_UI_DIR; creates temporary web directories with package.json for testing.
Web directory discovery and config loading regression tests
tests/cli/test_utils_coverage.py
Enhanced find_web_dir tests preventing accidental real-directory matches; tests for dist-only layout acceptance/rejection; regression test for load_full_config_from_db with DB-stored ui.enabled.
CLI test mocking updates for load_full_config_from_db
tests/cli/test_cli.py, tests/cli/test_cli_daemon.py, tests/cli/test_cli_extensions.py, tests/cli/test_cli_init.py, tests/cli/test_cli_install.py, tests/cli/test_install_prompts.py
Systematically updates all CLI command tests to patch gobby.cli.load_full_config_from_db instead of gobby.cli.load_config across status, init, install, uninstall, daemon, extension, and hook test cases.

Documentation and Version Updates

Layer / File(s) Summary
Version bump and release notes
pyproject.toml, src/gobby/__init__.py, CHANGELOG.md
Bumps version 0.4.1 → 0.4.2; comprehensive changelog documenting embedding customization, wheel UI packaging, MCP config migration, and test filtering.
Web UI port clarification in documentation
README.md, docs/guides/observability.md, docs/guides/providers-and-models.md, docs/guides/system-requirements.md, docs/guides/web-ui.md
Clarifies installed web UI runs on :60887 (with HTTP API and WebSocket), separate from dev UI on :60889; updates example URLs and quick-start instructions.
MCP config path and build asset docs, gitignore update
docs/guides/configuration.md, .gitignore
Documents new MCP path ~/.gobby/mcp-servers.json; documents new embedding install flags; adds .gitignore entry for built web UI assets.

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly Related Issues

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.48% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Release v0.4.2' directly matches the main objective of this PR, which is to cut and release version 0.4.2 into main.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 0.4.2

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link
Copy Markdown

claude Bot commented May 12, 2026

Code Review: Release v0.4.2

Overall impression: A solid release with meaningful additions — custom embedding endpoints, web UI packaging, and MCP config migration. The implementation is generally clean and the changelog/README updates are comprehensive. A few issues worth addressing before merge, mostly around error-handling robustness and test coverage.


🔴 High Priority

1. Broad exception swallowing in _probe_embedding_dim (src/gobby/cli/installers/embedding.py)

# Current
except Exception as e:
    logger.warning(f"Embedding dim probe failed: {e}")
    return None

Catching bare Exception hides real failures (network errors, auth failures, invalid models). Per project conventions, specific exception types should be caught. Suggest catching httpx.HTTPError, asyncio.TimeoutError, etc. and re-raising unexpected ones.

Also, the outer RuntimeError check using string matching is fragile:

if "cannot be called from a running event loop" in str(e):

This ties the code to CPython's current error message wording. Better to use asyncio.get_event_loop().is_running() before calling asyncio.run().

2. Silent failure on probe → install abort (embedding.py, lines 738–752)

When only --embedding-url is provided (no --embedding-dim), a probe is attempted. If the probe fails, the entire install aborts:

elif model_override is not None or api_base_override is not None:
    probed = _probe_embedding_dim(...)
    if probed is None:
        return {"success": False, "error": "..."}  # hard fail

The expected behavior should be to fall back to cfg["dim"] from the selected provider config, not fail entirely. A user supplying a custom URL with a compatible model shouldn't need to also specify the dim just because their endpoint is temporarily unreachable.

3. Opaque subprocess failures in build_backend/__init__.py

subprocess.run(["npm", "ci"], cwd=_WEB_SRC, check=True)
subprocess.run(["npm", "run", "build"], cwd=_WEB_SRC, check=True)

On failure, these produce a bare CalledProcessError with no context about what npm printed. Add capture_output=True and re-raise with stdout/stderr included so CI logs are actionable.


🟡 Medium Priority

4. _infer_embedding_provider_from_url is too simplistic (src/gobby/cli/_install_prompts.py, line 525)

def _infer_embedding_provider_from_url(api_base: str) -> str:
    return "ollama" if ":11434" in api_base else "lmstudio"

Port-based inference is fragile — Ollama and LM Studio can each run on custom ports. More importantly, http://api.openai.com/v1 would be labeled "lmstudio". This label affects which provider config path is used. Consider accepting an explicit --embedding-provider CLI arg or defaulting to a generic OpenAI-compatible profile for unrecognized URLs.

5. migrate_legacy_mcp_config edge cases untested (src/gobby/config/mcp.py)

Tests cover the happy path (legacy exists, new doesn't) but not:

  • Both files already exist (currently logs a warning and no-ops — is that the right behavior?)
  • Corrupted JSON in the legacy file
  • Permission denied on rename

These are easy to test with tmp_path and are the cases most likely to fail in the wild.

6. No integration test for wheel → UI serving path

CI now verifies gobby/ui/web/dist/index.html is present in the wheel (good), but there's no test that:

  1. Installs the wheel
  2. Starts the daemon
  3. Confirms the UI is actually served at :60887

This is the critical path for the feature and should have at least a smoke test.


🟢 Low Priority / Observations

7. Type hints missing on build_backend module-level constants

Per CLAUDE.md, all functions require type hints. Module-level Path constants (_REPO_ROOT, _WEB_SRC, etc.) don't have explicit type annotations. Minor, but consistent with project style.

8. Voice test exclusion lacks explanation

--ignore=tests/voice --ignore=tests/servers/routes/test_voice_routes.py

No comment explaining why. Future contributors won't know if this is intentional (heavy deps, flaky) or an oversight. A brief inline comment or a note in the CI job name would help.

9. MCP migration should be more visible to users

migrate_legacy_mcp_config() logs at WARNING level and silently migrates. Consider logging at INFO when migration succeeds (e.g., "Migrated ~/.gobby/.mcp.json → ~/.gobby/mcp-servers.json") so users know their config was touched. No cleanup of the legacy file is done — worth documenting that the old file can be removed.

10. GOBBY_SKIP_UI_BUILD is undocumented

The env var is used in build_backend/__init__.py but doesn't appear in docs/guides/configuration.md or the README. Worth a one-liner note since it affects reproducibility in dev environments.


Positive Notes

  • Changelog is thorough and well-organized — references to the specific GitHub issues being fixed are helpful.
  • The wheel verification step in CI (gobby/ui/web/dist/index.html) is a nice guard against packaging regressions.
  • MCP config migration is idempotent and non-destructive — good defensive design.
  • Embedding installer tests are comprehensive for the happy paths (11 new test cases).
  • Using find_web_dir() with a require_source flag is a clean abstraction for separating dev vs. installed UI lookup.

Summary

Area Status
Error handling ⚠️ Needs improvement (broad exceptions, probe failure → hard abort)
Type hints ✅ Mostly good, minor gaps in build_backend
Security ✅ No significant concerns
Test coverage ⚠️ Missing integration test for wheel serving, edge cases for migration
Documentation ✅ Good, minor gaps (GOBBY_SKIP_UI_BUILD, voice exclusion rationale)
Project conventions ✅ Mostly followed

The high-priority items (#1#3) are the most important to address. The rest are polish items that could be handled in follow-up tasks if needed.

Review by Claude Sonnet 4.6

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Release merge for v0.4.2, focused on (1) shipping production Web UI assets inside built wheels, (2) adding custom embedding endpoint/model/dim install support, and (3) renaming/migrating the global MCP server registry file to avoid .mcp.json schema collisions.

Changes:

  • Add a custom PEP 517 build backend to stage web/dist into the Python package and verify wheels contain gobby/ui/web/dist/index.html.
  • Extend gobby install + installer prompts to support --embedding-url, --embedding-model, --embedding-dim (including dim probing).
  • Rename ~/.gobby/.mcp.json~/.gobby/mcp-servers.json with idempotent migration; update CLI config loading to honor DB config_store values.

Reviewed changes

Copilot reviewed 35 out of 37 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
uv.lock Bump local workspace version to 0.4.2.
pyproject.toml Version bump; switch to build_backend; include UI dist in package-data.
build_backend/init.py Custom PEP 517 backend to build/stage UI and verify wheel contents.
MANIFEST.in Ensure build_backend is included in sdists.
.gitignore Ignore staged UI assets under src/gobby/ui/.
src/gobby/init.py Bump __version__ to 0.4.2.
src/gobby/cli/init.py Load CLI config via load_full_config_from_db (DB-aware).
src/gobby/cli/utils.py Add load_full_config_from_db(config_file=...); expand find_web_dir() to accept dist-only installs.
src/gobby/cli/ui.py Resolve web/ via find_web_dir() for dev/build/install-deps flows.
src/gobby/cli/install.py Add embedding override CLI flags and pass overrides into embedding installer prompts.
src/gobby/cli/_install_prompts.py Add embedding override prompting + provider inference from custom URL.
src/gobby/cli/installers/embedding.py Support URL/model/dim overrides; add dim probing helper.
src/gobby/cli/installers/mcp_config.py Write default MCP servers to mcp-servers.json and run legacy migration.
src/gobby/config/mcp.py New default MCP config path, constants, and migration helper.
src/gobby/runner_lifecycle.py Run MCP legacy migration on daemon startup.
README.md Update UI port docs (installed UI on :60887; dev UI on :60889).
docs/guides/web-ui.md Document installed UI served from daemon port and dev UI behavior.
docs/guides/system-requirements.md Update port ownership and UI requirements.
docs/guides/providers-and-models.md Update UI URLs to :60887.
docs/guides/observability.md Update dashboard/traces URLs to :60887.
docs/guides/configuration.md Document MCP path rename + embedding override flags and probing behavior.
CHANGELOG.md Add 0.4.2 release notes.
.github/workflows/ci.yml Exclude voice tests from main run; add wheel UI index assertion in package check.
.github/workflows/release.yml Exclude voice tests from pre-release test run.
tests/test_build_backend.py Unit tests for staging UI and wheel verification behavior.
tests/servers/test_app_factory_production_ui.py Regression tests for production UI mounting with dist-only layout.
tests/config/test_mcp_config.py Update MCP default path expectations + migration tests.
tests/cli/test_utils_coverage.py Update find_web_dir tests + add regression test for DB-backed ui.enabled.
tests/cli/test_ui_coverage.py Update UI CLI tests to use find_web_dir and Click context config.
tests/cli/test_install_prompts.py Add embedding override args to install prompt test expectations.
tests/cli/test_install_embedding_wizard.py Add coverage for embedding overrides and interactive customize flow.
tests/cli/installers/test_embedding_installer.py Add tests for override behavior, dim probing, and setup skipping.
tests/cli/test_cli.py Update CLI tests to patch load_full_config_from_db.
tests/cli/test_cli_install.py Update install/uninstall CLI tests to patch load_full_config_from_db.
tests/cli/test_cli_init.py Update init CLI tests to patch load_full_config_from_db.
tests/cli/test_cli_extensions.py Update extensions CLI tests to patch load_full_config_from_db.
tests/cli/test_cli_daemon.py Update daemon CLI tests to patch load_full_config_from_db.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/gobby/cli/utils.py
Comment on lines +506 to +513
def _qualifies(p: Path) -> bool:
if not p.exists():
return False
if (p / "package.json").exists():
return True
if not require_source and (p / "dist" / "index.html").exists():
return True
return False
Comment thread src/gobby/cli/install.py
@click.option(
"--embedding-dim",
"embedding_dim",
type=int,
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@build_backend/__init__.py`:
- Around line 73-74: In _stage_ui(), the two subprocess.run calls that invoke
npm (the lines calling subprocess.run(["npm", "ci"], ...) and
subprocess.run(["npm", "run", "build"], ...)) lack timeouts; add a timeout
argument (e.g. timeout=600) to each call and wrap them in a try/except that
catches subprocess.TimeoutExpired to log or raise a clear error (propagate a
RuntimeError or re-raise with context) so CI won't hang indefinitely if npm
stalls. Ensure you update both calls in the _stage_ui() function and include
contextual information (command and timeout) in the error handling.

In `@pyproject.toml`:
- Around line 113-115: Update pyproject.toml to raise the setuptools minimum
requirement from 61.0 to 64.0 because build_backend.build_editable() delegates
to setuptools.build_meta.build_editable which was added in setuptools 64.0.
Locate the requires = ["setuptools>=61.0", "wheel"] entry and change the
setuptools version to >=64.0 so editable installs using
build_backend.build_editable() will work in constrained build environments.

In `@src/gobby/cli/install.py`:
- Around line 165-170: The CLI currently accepts non-positive values for the
"--embedding-dim" argument; modify the add_argument calls that define
"--embedding-dim" (the entries setting dest "embedding_dim") to validate input
at the argparse boundary by using a custom type/validator (e.g., positive_int)
that raises argparse.ArgumentTypeError for values <= 0 so users get immediate
feedback; apply the same change to the other add_argument defining
"--embedding-dim" referenced in the file (the second occurrence around the block
at 197-202) so both CLI entry points reject zero/negative integers.

In `@src/gobby/config/mcp.py`:
- Around line 63-66: Wrap the call to legacy.replace(new) in an OSError handler
so migration is best-effort: attempt the replace inside try/except OSError, and
on exception log a warning with the exception (using logger.warning or
logger.exception) and fall back to a copy+unlink move strategy (e.g.,
shutil.copy2(legacy, new) then legacy.unlink()) before proceeding; after a
successful move (either replace or fallback) set restrictive permissions via
new.chmod(0o600) and only return True on success, otherwise return False so
initialization can continue safely. Ensure you reference and update the existing
new.parent.mkdir(new.parent.mkdir(...)), legacy.replace(new), logger.info(...)
locations and add new.chmod(0o600) after the move and appropriate exception
logging and fallback logic.

In `@tests/cli/test_ui_coverage.py`:
- Line 328: The current positional-only assertion is brittle; instead read the
recorded call from mock_find.call_args and pick up the config either from
call_args.kwargs['config'] if present or from call_args.args[0] otherwise, then
assert the call used require_source=True by invoking
mock_find.assert_called_with(config, require_source=True) (use the symbols
mock_find, call_args and assert_called_with to locate and implement this
change).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 45ef7d2d-d782-4b0d-94e0-2bed3d2b1797

📥 Commits

Reviewing files that changed from the base of the PR and between 62f2738 and 3528ab8.

⛔ Files ignored due to path filters (1)
  • uv.lock is excluded by !**/*.lock
📒 Files selected for processing (36)
  • .github/workflows/ci.yml
  • .github/workflows/release.yml
  • .gitignore
  • CHANGELOG.md
  • MANIFEST.in
  • README.md
  • build_backend/__init__.py
  • docs/guides/configuration.md
  • docs/guides/observability.md
  • docs/guides/providers-and-models.md
  • docs/guides/system-requirements.md
  • docs/guides/web-ui.md
  • pyproject.toml
  • src/gobby/__init__.py
  • src/gobby/cli/__init__.py
  • src/gobby/cli/_install_prompts.py
  • src/gobby/cli/install.py
  • src/gobby/cli/installers/embedding.py
  • src/gobby/cli/installers/mcp_config.py
  • src/gobby/cli/ui.py
  • src/gobby/cli/utils.py
  • src/gobby/config/mcp.py
  • src/gobby/runner_lifecycle.py
  • tests/cli/installers/test_embedding_installer.py
  • tests/cli/test_cli.py
  • tests/cli/test_cli_daemon.py
  • tests/cli/test_cli_extensions.py
  • tests/cli/test_cli_init.py
  • tests/cli/test_cli_install.py
  • tests/cli/test_install_embedding_wizard.py
  • tests/cli/test_install_prompts.py
  • tests/cli/test_ui_coverage.py
  • tests/cli/test_utils_coverage.py
  • tests/config/test_mcp_config.py
  • tests/servers/test_app_factory_production_ui.py
  • tests/test_build_backend.py

Comment thread build_backend/__init__.py
Comment on lines +73 to +74
subprocess.run(["npm", "ci"], cwd=_WEB_SRC, check=True) # nosec B603 B607
subprocess.run(["npm", "run", "build"], cwd=_WEB_SRC, check=True) # nosec B603 B607
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add timeouts to npm subprocess calls in _stage_ui().

These build-critical external calls currently have no timeout, so a network/npm stall can block CI/release indefinitely.

Suggested fix
-        subprocess.run(["npm", "ci"], cwd=_WEB_SRC, check=True)  # nosec B603 B607
-        subprocess.run(["npm", "run", "build"], cwd=_WEB_SRC, check=True)  # nosec B603 B607
+        subprocess.run(["npm", "ci"], cwd=_WEB_SRC, check=True, timeout=600)  # nosec B603 B607
+        subprocess.run(
+            ["npm", "run", "build"], cwd=_WEB_SRC, check=True, timeout=600
+        )  # nosec B603 B607
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
subprocess.run(["npm", "ci"], cwd=_WEB_SRC, check=True) # nosec B603 B607
subprocess.run(["npm", "run", "build"], cwd=_WEB_SRC, check=True) # nosec B603 B607
subprocess.run(["npm", "ci"], cwd=_WEB_SRC, check=True, timeout=600) # nosec B603 B607
subprocess.run(
["npm", "run", "build"], cwd=_WEB_SRC, check=True, timeout=600
) # nosec B603 B607
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@build_backend/__init__.py` around lines 73 - 74, In _stage_ui(), the two
subprocess.run calls that invoke npm (the lines calling subprocess.run(["npm",
"ci"], ...) and subprocess.run(["npm", "run", "build"], ...)) lack timeouts; add
a timeout argument (e.g. timeout=600) to each call and wrap them in a try/except
that catches subprocess.TimeoutExpired to log or raise a clear error (propagate
a RuntimeError or re-raise with context) so CI won't hang indefinitely if npm
stalls. Ensure you update both calls in the _stage_ui() function and include
contextual information (command and timeout) in the error handling.

Comment thread pyproject.toml
Comment on lines 113 to +115
requires = ["setuptools>=61.0", "wheel"]
build-backend = "setuptools.build_meta"
build-backend = "build_backend"
backend-path = ["."]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

What is the first setuptools version that provides setuptools.build_meta.build_editable (PEP 660)?

💡 Result:

The first setuptools version that provides the PEP 660 hook setuptools.build_meta.build_editable is setuptools 64.0.0.[1][2] Setuptools’ NEWS notes that “#3265: Added implementation for editable install hooks (PEP 660)”.[2] That change was included starting with the 64.0.0 release line (hence first version with build_editable support is 64.0.0).[2][3]

Citations:


🏁 Script executed:

fd -name "build_backend.py" -o -name "build_backend" -type f

Repository: GobbyAI/gobby

Length of output: 288


🏁 Script executed:

rg "build_editable|build_meta" --type py

Repository: GobbyAI/gobby

Length of output: 744


🏁 Script executed:

cat -n build_backend/__init__.py

Repository: GobbyAI/gobby

Length of output: 5173


🏁 Script executed:

head -120 pyproject.toml | tail -10

Repository: GobbyAI/gobby

Length of output: 248


Raise the setuptools minimum to 64.0 to support the PEP 660 editable build hook.

build_backend.build_editable() (lines 122-127) delegates to setuptools.build_meta.build_editable, which was first provided in setuptools 64.0.0. The current requirement of setuptools>=61.0 is insufficient and will cause editable installs to fail in constrained build environments.

Suggested change
[build-system]
-requires = ["setuptools>=61.0", "wheel"]
+requires = ["setuptools>=64.0", "wheel"]
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
requires = ["setuptools>=61.0", "wheel"]
build-backend = "setuptools.build_meta"
build-backend = "build_backend"
backend-path = ["."]
requires = ["setuptools>=64.0", "wheel"]
build-backend = "build_backend"
backend-path = ["."]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyproject.toml` around lines 113 - 115, Update pyproject.toml to raise the
setuptools minimum requirement from 61.0 to 64.0 because
build_backend.build_editable() delegates to setuptools.build_meta.build_editable
which was added in setuptools 64.0. Locate the requires = ["setuptools>=61.0",
"wheel"] entry and change the setuptools version to >=64.0 so editable installs
using build_backend.build_editable() will work in constrained build
environments.

Comment thread src/gobby/cli/install.py
Comment on lines +165 to +170
"--embedding-dim",
"embedding_dim",
type=int,
default=None,
help="Override the embedding dimension. Omit to auto-detect via /v1/embeddings probe.",
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Validate --embedding-dim as a positive integer at the CLI boundary.

Right now 0 or negative values are accepted and only fail downstream (or produce unclear behavior). Rejecting invalid values here gives immediate, actionable feedback.

Suggested fix
 def install(
@@
 ) -> None:
@@
+    if embedding_dim is not None and embedding_dim <= 0:
+        click.echo("--embedding-dim must be a positive integer", err=True)
+        sys.exit(2)
+
     if (

Also applies to: 197-202

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/gobby/cli/install.py` around lines 165 - 170, The CLI currently accepts
non-positive values for the "--embedding-dim" argument; modify the add_argument
calls that define "--embedding-dim" (the entries setting dest "embedding_dim")
to validate input at the argparse boundary by using a custom type/validator
(e.g., positive_int) that raises argparse.ArgumentTypeError for values <= 0 so
users get immediate feedback; apply the same change to the other add_argument
defining "--embedding-dim" referenced in the file (the second occurrence around
the block at 197-202) so both CLI entry points reject zero/negative integers.

Comment thread src/gobby/config/mcp.py
Comment on lines +63 to +66
new.parent.mkdir(parents=True, exist_ok=True)
legacy.replace(new)
logger.info("Migrated MCP config %s -> %s", legacy, new)
return True
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Harden migration failure path and enforce restrictive permissions.

On Lines 64-65, legacy.replace(new) can raise OSError (permissions, FS issues) and currently aborts initialization. Also, migrated file mode may remain too permissive. Make migration best-effort and set mode to 0o600 after move.

Suggested patch
     new.parent.mkdir(parents=True, exist_ok=True)
-    legacy.replace(new)
-    logger.info("Migrated MCP config %s -> %s", legacy, new)
-    return True
+    try:
+        legacy.replace(new)
+        new.chmod(0o600)
+    except OSError as e:
+        logger.warning("Failed to migrate MCP config %s -> %s: %s", legacy, new, e)
+        return False
+
+    logger.info("Migrated MCP config %s -> %s", legacy, new)
+    return True
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/gobby/config/mcp.py` around lines 63 - 66, Wrap the call to
legacy.replace(new) in an OSError handler so migration is best-effort: attempt
the replace inside try/except OSError, and on exception log a warning with the
exception (using logger.warning or logger.exception) and fall back to a
copy+unlink move strategy (e.g., shutil.copy2(legacy, new) then legacy.unlink())
before proceeding; after a successful move (either replace or fallback) set
restrictive permissions via new.chmod(0o600) and only return True on success,
otherwise return False so initialization can continue safely. Ensure you
reference and update the existing new.parent.mkdir(new.parent.mkdir(...)),
legacy.replace(new), logger.info(...) locations and add new.chmod(0o600) after
the move and appropriate exception logging and fallback logic.

result = runner.invoke(ui, ["dev"], obj={"config": MagicMock()}, catch_exceptions=False)
assert result.exit_code == 0
assert "Starting dev server" in result.output
mock_find.assert_called_with(mock_find.call_args.args[0], require_source=True)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Make the find_web_dir assertion resilient to arg-style changes.

The current check depends on positional args and can break if config is passed as a keyword while behavior remains correct.

Proposed tweak
-        mock_find.assert_called_with(mock_find.call_args.args[0], require_source=True)
+        mock_find.assert_called()
+        assert mock_find.call_args is not None
+        assert mock_find.call_args.kwargs.get("require_source") is True
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/cli/test_ui_coverage.py` at line 328, The current positional-only
assertion is brittle; instead read the recorded call from mock_find.call_args
and pick up the config either from call_args.kwargs['config'] if present or from
call_args.args[0] otherwise, then assert the call used require_source=True by
invoking mock_find.assert_called_with(config, require_source=True) (use the
symbols mock_find, call_args and assert_called_with to locate and implement this
change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants